Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

app.go cleanup #1580

Merged
merged 30 commits into from
Oct 10, 2022
Merged

app.go cleanup #1580

merged 30 commits into from
Oct 10, 2022

Conversation

Anmol1696
Copy link
Contributor

PR for #1557

Cleanup to make app.go easier and more similar to cleaner upgrades and keeper

  • upgrades/ directory to hold various chain upgrade functions
  • keepers/ dir to hold all keeper creation, just moved keeper defination from app.go to keeper and another struct to be passed to upgrades.

Note: Core logic for is just moved to new structure, not much refactor.

@okwme
Copy link
Contributor

okwme commented Jul 11, 2022

Thank you @Anmol1696 !
appreciate the new formatting : )
will need to check on status of new features being added to main to confirm whether to merge this before or after (global fee + bypass min fee specifically)
please standby!

@yaruwangway
Copy link
Contributor

probably after global fee and ica auth module integrated...

@Anmol1696
Copy link
Contributor Author

Anmol1696 commented Jul 11, 2022

Cool no problem, will wait till then before resolving all following conflicts.

Let me know if there is anything else to be added to this PR.

@Anmol1696
Copy link
Contributor Author

I guess still waiting on some of the PRs. These are the PRs i should be waiting for right?

@yaruwangway
Copy link
Contributor

I guess still waiting on some of the PRs. These are the PRs i should be waiting for right?

@faddat
Copy link
Contributor

faddat commented Aug 1, 2022

I like this PR.

I'd like to take some time tomorrow, and compare it to Osmosis' app.go, I really like how they've structured things.

Copy link
Contributor

@faddat faddat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this style, and have been working with it at Osmosis.

I ran through the PR just now and overall, I just think that it'll benefit us re: usability and readability.

I am also sure that it's going to need a little bit of restructuring because of the upgrade 46. Would you like to do it @Anmol1696 or should I pull your branch and make the changes in a new branch?

@Anmol1696
Copy link
Contributor Author

@faddat i can take up the restructure, if you can point me towards it, i want to take it up.

@okwme okwme mentioned this pull request Aug 15, 2022
5 tasks
@faddat
Copy link
Contributor

faddat commented Aug 25, 2022

So here are two examples:

https://github.com/notional-labs/craft

https://github.com/osmosis-labs/osmosis

They are both very similar, and I think it is a great way to reduce the complexity of app.go

app/keepers/keepers.go Outdated Show resolved Hide resolved
@yaruwangway
Copy link
Contributor

@Anmol1696 , please solve the conflicts and we can review and merge the pr ! thank you! 👍

@Anmol1696
Copy link
Contributor Author

Anmol1696 commented Oct 5, 2022

The complaint seems to be directly for the app/app.go file itself, ignoring other files does not seem to help this case. I would not want to ignore the app.go file itself for this though. I did try to ignore the files and it does not seem to help.

This seems to be special case where there is an overhaul of the full app.go file, hence the CI for having 5% change does not make sense (for this PR only though).

If we dont want to use admin to merge the PR, we could have back to back PRs, in this PR we ignore the app.go, and after it is merged, next PR to remove the ignore.

Let me know how you want me to proceed here @yaruwangway, or if you have any other suggestions.

Copy link
Contributor

@Pantani Pantani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can remove the EmptyAppOptions from the app.go. It is duplicated in the helpers/test_helpers.go

app/app.go Show resolved Hide resolved
@yaruwangway
Copy link
Contributor

@mmulji-ic can we force merge ?

@okwme
Copy link
Contributor

okwme commented Oct 5, 2022

ah sorry @Anmol1696, every PR that gets merged into main presents a new set of conflicts. If you could help get the latest ones resolved I'll merge the PR before the next ones, thank you!

@Anmol1696
Copy link
Contributor Author

Yup let me take it up today and finish this up.
Thanks @yaruwangway for the lint fixes.

@yaruwangway
Copy link
Contributor

Hi @Anmol1696, thank you! Presently, i did not merge this commit of bumping ibc in your PR, you can neglect this commit, we can do this commit again after your PR merged!

@yaruwangway
Copy link
Contributor

@Anmol1696
you can just comment out TestAccAddress = icatypes.GenerateAddress(sdk.Context{}, ibctesting.FirstConnectionID, TestPortID) to fix the err. this is not even used.

@Anmol1696
Copy link
Contributor Author

Hmm somehow the commits got messed up, i tried to hard push a force reset to 2e231b7 on my branch and a commit to comment out the test function in the latest commit, but now when i checkout to 2e231b7, the build still seems to be broken, although main itself i building.

A little confused here.

@Anmol1696
Copy link
Contributor Author

It seems i can try to do the merge again though and see what happens on my branch, but l am not sure what just happened here to this PR

@yaruwangway
Copy link
Contributor

It seems i can try to do the merge again though and see what happens on my branch, but l am not sure what just happened here to this PR

so you force pushed? I think there are two commits are missing. i can fix it.

@Anmol1696
Copy link
Contributor Author

well i forced push to remove your commits from my branch, but turns out [2e231b7] commit itself was broken.. i am just curious how it got there now. Force pushing to remove unwanted commits basically

@yaruwangway
Copy link
Contributor

you only removed two commits from mine, there are three.

@yaruwangway
Copy link
Contributor

Would you like to fix this PR or you prefer I do ?

@Anmol1696
Copy link
Contributor Author

yup i was asking since that was a merge commit to update my branch with main, i kept it.
I will remove that commit as well then, but my question was around that only, how was a merge commit wrong.

@Anmol1696
Copy link
Contributor Author

well the fix just requiers a force push back to orginial and then re-merge the main branch. Right?
I will do it

@yaruwangway
Copy link
Contributor

yaruwangway commented Oct 7, 2022

ha, if you mean why merge failed the test,
this is due to this commit of bumping ibc in main.

from present commit, you can further commit to convert back the ibc-go version from v5.0.0 to v5.0.0-rc2. it will fix some of the tests. and we will bump the ibc again after your branch merge.

otherwise, you can reset back to 1cfbbb446905c0a02652aa579af60df9fed4180f and re-merge main. mainly you need to distribute this commit of bumping ibc into the refactor.

@Anmol1696
Copy link
Contributor Author

Ahh cool cool, my thought was that the merge of main branch was correct, because this bump of ibc was in a later commit of fix linting which i rolled back as well.
Let me just reset and merge main branch completly then. Thanks

@Anmol1696
Copy link
Contributor Author

Done with the branch again. can you guys have a look: @yaruwangway @okwme

@yaruwangway yaruwangway merged commit 3438eda into cosmos:main Oct 10, 2022
@yaruwangway
Copy link
Contributor

merged! thanks! @Anmol1696

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants